Skip to content

Conversation

@olehbozhok
Copy link
Contributor

@olehbozhok olehbozhok commented Feb 2, 2026

Prepare


Description

Added loading trusted issuers on basctround after startup. If user specify bootstrap parameter.

Target issue

closes #10004

Implementation Details

  • fixed some cargo problems
  • added ValueObject IssClaim (store normalized value of iss).
    • it allows easier to track iss claim value over the code and remove unnecessary normalization checks.
  • added async background loading trusted issuer
    • CEDARLING_TRUSTED_ISSUER_LOADER_TYPE
    • CEDARLING_TRUSTED_ISSUER_LOADER_WORKERS
  • removed unused code

Test and Document the changes

  • Static code analysis has been run locally and issues have been fixed
  • Relevant unit and integration tests have been added/updated
  • Relevant documentation has been updated if any (i.e. user guides, installation and configuration guides, technical design docs etc)

Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with docs: to indicate documentation changes or if the below checklist is not selected.

  • I confirm that there is no impact on the docs due to the code changes in this PR.

Summary by CodeRabbit

  • New Features

    • Configurable trusted-issuer loader with sync/async modes and worker-count control (default 1).
    • OpenID configuration can include a status_list_endpoint for status-list validation.
  • Documentation

    • Bootstrap properties updated with explicit default-value annotations and minor formatting tweaks.
  • Improvements

    • Enhanced concurrency and thread-safety for JWT validation, key management, and issuer indexing.
    • More robust and consistent issuer handling and normalization across token flows.

…ucture

Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…ized iss value

Using this structure easier to understand where actual iss value is used.

Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…SSUER_LOADER_ASYNC`

Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…ng trusted issuers. But only sync version

Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…issuers`

Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
on command `cargo clippy --tests`

Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…ypes

now user can use `CEDARLING_TRUSTED_ISSUER_LOADER_TYPE` and `CEDARLING_TRUSTED_ISSUER_LOADER_WORKERS` parameters

Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…ing fields and defaults

Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 2, 2026

Caution

Review failed

An error occurred during the review process. Please try again later.

📝 Walkthrough

Walkthrough

Adds an IssClaim newtype and migrates issuer handling to it, makes multiple JWT/key/status-list caches thread-safe with Arc/RwLock, introduces a TrustedIssuerLoader (sync/async) configurable via bootstrap, tightens policy-store visibility, and updates many modules and tests to use IssClaim and Arc-wrapped keys.

Changes

Cohort / File(s) Summary
Docs & Test Guidance
docs/cedarling/reference/cedarling-properties.md, jans-cedarling/AGENTS.md
Adds explicit default notes for bootstrap properties and updates test assertion patterns/messages and file header example.
Cargo / Bootstrap Config
jans-cedarling/cedarling/Cargo.toml, jans-cedarling/cedarling/src/bootstrap_config/...
Enables ahash "std" feature; adds trusted_issuer_loader_type and trusted_issuer_loader_workers to raw config and wires loader config into JwtConfig.
Issuer Utilities
jans-cedarling/cedarling/src/common/issuer_utils.rs
Introduces IssClaim newtype (Serialize/Deserialize), makes normalize_issuer private, adds tests and (de)serializers.
Policy Store Visibility
jans-cedarling/cedarling/src/common/policy_store.rs, .../claim_mapping.rs, .../token_entity_metadata.rs
Reduces public visibility to pub(crate) for several policy types/methods, adds iss_claim() on TrustedIssuer, removes test builders, and adjusts field visibilities and test helpers.
Entity Builder & Indexing
jans-cedarling/cedarling/src/entity_builder/...
Switches callers to TrustedIssuer::new/accessors, uses IssClaim for issuer extraction and entity-key generation, and updates tests.
JWT HTTP & OpenID
jans-cedarling/cedarling/src/jwt/http_utils.rs
Changes OpenIdConfig.issuer to IssClaim, adds status_list_endpoint, and conditions async_trait attributes for wasm/non-wasm targets.
Issuer Index & Loader
jans-cedarling/cedarling/src/jwt/issuer_index.rs, .../trusted_issuers_loader.rs
Adds thread-safe IssuerIndex (RwLock map) and a new TrustedIssuerLoader module implementing sync/async loading, concurrency control, OpenID/JWKS fetching, validator/status-list init, and extensive tests.
Key Service & Test Utils
jans-cedarling/cedarling/src/jwt/key_service.rs, .../test_utils.rs
Converts key storage to RwLock<HashMap<..., Arc<DecodingKey>>>, changes APIs from &mut self to &self where appropriate, and updates tests/mocks to use Arc<DecodingKey> and IssClaim.
JWT Core, Token & Cache
jans-cedarling/cedarling/src/jwt/mod.rs, .../token.rs, .../token_cache.rs
Wraps core services in Arc, replaces issuer maps with IssuerIndex, integrates TrustedIssuerLoader, returns IssClaim from token extraction, and uses IssClaim in cache keys.
Validator & Validator Cache
jans-cedarling/cedarling/src/jwt/validation/...
Changes validator APIs to accept Option<&IssClaim>, makes JwtValidatorCache RwLock-backed, adopts OwnedValidatorInfo with IssClaim, and changes multi-issuer TokenKind to use Cow for borrowing.
Status List & TrustedIssuerValidator
jans-cedarling/cedarling/src/jwt/status_list/cache.rs, .../trusted_issuer_validator.rs
Makes status-list init take &self, accepts Arc<DecodingKey> in updaters, and refactors TrustedIssuerValidator to key by IssClaim.
Lock Module
jans-cedarling/cedarling/src/lock/mod.rs
Uses IssClaim when constructing JWKS URIs from issuer OIDC URL.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related issues

  • #10004 — Background loading of Trusted IDP metadata: this PR implements a TrustedIssuerLoader with sync/async modes matching that objective.
  • JanssenProject/jans#13126 — Adds a TrustedIssuerLoader and background loading behavior that closely align with changes here.

Possibly related PRs

  • JanssenProject/jans#12982 — Related changes to issuer normalization/representation; overlaps with IssClaim introduction.
  • JanssenProject/jans#12896 — Reworks trusted-issuer indexing; relates to the new IssuerIndex and its callers.
  • JanssenProject/jans#12956 — Touches issuer/JWT codepaths and likely overlaps with validator/key changes here.

Suggested reviewers

  • dagregi
  • haileyesus2433
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description check ✅ Passed The PR description covers required sections: preparation checklist completed, clear target issue reference (#10004), specific implementation details provided, and test/documentation status confirmed.
Linked Issues check ✅ Passed The code changes address issue #10004 requirements: implements lazy/background loading of trusted issuer metadata via IssClaim ValueObject, adds configurable async loader with CEDARLING_TRUSTED_ISSUER_LOADER_TYPE and CEDARLING_TRUSTED_ISSUER_LOADER_WORKERS, includes tests and documentation.
Out of Scope Changes check ✅ Passed All changes are within scope: introduction of IssClaim ValueObject, background loader implementation, visibility adjustments for internal APIs, thread-safe index structures, and supporting infrastructure are all necessary for the stated objective of background issuer loading.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check ✅ Passed The PR title accurately describes the main feature: background loading of trusted issuers on startup.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch jans-cedarling-10004

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mo-auto
Copy link
Member

mo-auto commented Feb 2, 2026

Error: Hi @olehbozhok, You did not reference an open issue in your PR. I attempted to create an issue for you.
Please update that issues' title and body and make sure I correctly referenced it in the above PRs body.

@mo-auto mo-auto added area-documentation Documentation needs to change as part of issue or PR comp-docs Touching folder /docs comp-jans-cedarling Touching folder /jans-cedarling kind-feature Issue or PR is a new feature request labels Feb 2, 2026
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
@mo-auto
Copy link
Member

mo-auto commented Feb 2, 2026

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
jans-cedarling/AGENTS.md (1)

91-93: ⚠️ Potential issue | 🟡 Minor

Fix spelling in JWT status validation description.
“retreive” → “retrieve”.

✅ Proposed fix
- On startup, the Cedarling should fetch and retreive the latest Status List JWT from the `.well-known/openid-configuration` via the `status_list_endpoint` claim and cache it.
+ On startup, the Cedarling should fetch and retrieve the latest Status List JWT from the `.well-known/openid-configuration` via the `status_list_endpoint` claim and cache it.
jans-cedarling/cedarling/src/jwt/http_utils.rs (1)

140-143: ⚠️ Potential issue | 🟡 Minor

Fix typos in error messages: "respose" → "response".

Lines 140 and 142 contain the typo "respose" which should be "response".

✏️ Proposed fix
-    #[error("failed to deserialize respose from JSON: {0}")]
+    #[error("failed to deserialize response from JSON: {0}")]
     JsonDeserializeResponse(#[source] reqwest::Error),
-    #[error("failed to read the respose text: {0}")]
+    #[error("failed to read the response text: {0}")]
     ReadTextResponse(#[source] reqwest::Error),
jans-cedarling/cedarling/src/jwt/validation/validator_cache.rs (1)

119-139: 🧹 Nitpick | 🔵 Trivial

Avoid panicking on poisoned RwLock in cache accessors.

expect will crash the service on lock poisoning. Consider returning a Result (or logging and skipping) so the caller can decide how to recover.
As per coding guidelines, use thiserror for custom error types and prefer Result<T, E> over panics with context in error messages.

🤖 Fix all issues with AI agents
In `@docs/cedarling/reference/cedarling-properties.md`:
- Around line 91-93: Correct the spelling of "retreive" to "retrieve" in the
documentation entry for CEDARLING_JWT_STATUS_VALIDATION; update the sentence
that reads "fetch and retreive the latest Status List JWT..." to "fetch and
retrieve the latest Status List JWT..." and scan the same file for other
occurrences of "retreive" to fix them as well.

In `@jans-cedarling/cedarling/src/bootstrap_config/decode.rs`:
- Line 13: Remove the unused import NonZeroU16 from the top-level use statement
in decode.rs: keep NonZeroUsize only (used for trusted_issuer_workers) and
delete NonZeroU16 to avoid the unused-import warning; locate the line importing
std::num::{NonZeroU16, NonZeroUsize} and change it to import only NonZeroUsize.

In `@jans-cedarling/cedarling/src/common/policy_store.rs`:
- Around line 314-316: The current iss_claim implementation uses Url::origin()
which strips path segments and breaks path-based issuers; change iss_claim to
feed the full OIDC endpoint (including path) into IssClaim::new by using the
OIDC endpoint's full string representation (e.g., oidc_endpoint.as_str() or
equivalent) instead of origin(). Update the iss_claim function so it constructs
IssClaim with the complete oidc_endpoint value to preserve path-based issuers.

In `@jans-cedarling/cedarling/src/jwt/issuer_index.rs`:
- Around line 21-32: The comments on the public methods of IssuerIndex should
use Rust doc comments; change the line comments above the functions new() and
insert() to use triple-slash documentation (///) so they consistently generate
API docs like the other methods (update the comment before pub(super) fn new()
-> Self and before pub(super) fn insert(&self, iss: IssClaim, config:
IssuerConfig)).
- Around line 42-58: The function signature for find_token_metadata_key uses an
unnecessary lifetime 'a on &self and the &str parameter; change its signature to
fn find_token_metadata_key(&self, entity_type_name: &str) -> Option<String> (no
explicit lifetimes) and update any references to match; the body can remain the
same since the return is an owned String and no additional lifetime annotations
are required.
- Around line 1-6: Add the required Apache 2.0 license header at the very top of
the issuer_index.rs file (above the first use statements); ensure you insert the
project's standard Apache-2.0 header/SPDX line with the appropriate year and
copyright/owner information so every source file begins with the license; you
can locate the file by the presence of symbols like IssuerConfig, IssClaim, and
TrustedIssuer in the imports.

In `@jans-cedarling/cedarling/src/jwt/mod.rs`:
- Around line 177-186: The local JWKS loading is gated by
loader.jwt_config.trusted_issuer_loader.is_sync(), causing JWKS to be skipped
when the loader is async; remove that sync-only guard or add an async-path
equivalent so jwks are inserted regardless of loader mode. Specifically, ensure
the logic around loader.jwt_config.trusted_issuer_loader.is_sync() is changed so
that when jwt_config.jwt_sig_validation is true and jwt_config.jwks.is_some()
and !key_service.has_keys(), you call key_service.insert_keys_from_str(jwks) (or
call it from the async loader branch) so local JWKS are loaded in both sync and
async loader modes.

In `@jans-cedarling/cedarling/src/jwt/trusted_issuers_loader.rs`:
- Line 99: The code uses errors_clone.lock().unwrap() which will panic if the
mutex is poisoned; change the locking call in the error-collection path to
recover from poisoning by using errors_clone.lock().unwrap_or_else(|e|
e.into_inner()), so the inner Vec can still be accessed and errors pushed even
after a previous panic; update the site where
errors_clone.lock().unwrap().push(error) is used to call unwrap_or_else(|e|
e.into_inner()) before pushing.

In `@jans-cedarling/cedarling/src/jwt/validation/trusted_issuer_validator.rs`:
- Around line 73-84: The loop in TrustedIssuerValidator::new binds an unused
_id; remove it and simplify the iteration by mapping or discarding the id
directly—e.g., use trusted_issuers.into_iter().map(|(_, ti)| Arc::new(ti)) or
trusted_issuers.into_values().map(Arc::new) and iterate over each issuer, then
insert issuer.iss_claim() -> issuer into url_to_issuer; keep references to the
new function (new), the url_to_issuer variable, the TrustedIssuer type and the
iss_claim() method to locate the change.

In `@jans-cedarling/cedarling/src/jwt/validation/validator_cache.rs`:
- Around line 279-285: The issuer comparison in ValidatorInfo::is_equal_to
currently unwraps and compares string values, treating Some/None mismatches as
equal; change the check to compare the Option values directly (i.e., compare
self.iss with other.iss) so that Some(x) != None is detected and the function
returns false on mismatch; update the is_equal_to implementation to use direct
Option equality for the iss field (and analogous Option fields if present) to
avoid false positives from hash collisions.

Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Fix all issues with AI agents
In `@docs/cedarling/reference/cedarling-properties.md`:
- Line 63: The phrasing in the documentation for the
CEDARLING_TRUSTED_ISSUER_LOADER_WORKERS property contains a split infinitive
("used to asynchronously load"); update the sentence to a more traditional style
by moving the adverb, e.g., change "used to asynchronously load trusted issuers"
to "used to load trusted issuers asynchronously" while preserving the rest of
the description and the property name CEDARLING_TRUSTED_ISSUER_LOADER_WORKERS.
- Around line 61-64: Update the CEDARLING_TRUSTED_ISSUER_LOADER_WORKERS property
description to state it applies to both CEDARLING_TRUSTED_ISSUER_LOADER_TYPE
modes (SYNC and ASYNC): explain that in SYNC mode the worker count parallelizes
loading of trusted issuers during initialization (blocking startup), and in
ASYNC mode it parallelizes background loading (non-blocking); keep the
default/zero behavior note and mention default of 1.

In `@jans-cedarling/cedarling/src/bootstrap_config/decode.rs`:
- Around line 117-118: Replace the unsafe construction for
trusted_issuer_workers: instead of using NonZeroUsize::new_unchecked(1) in the
unwrap_or_else closure, call the safe constructor NonZeroUsize::new(1).expect("1
is non-zero") so the fallback remains a guaranteed non-zero value without unsafe
code; update the closure where trusted_issuer_workers is assigned (the
expression using raw.trusted_issuer_loader_workers and
NonZeroUsize::new_unchecked).

In `@jans-cedarling/cedarling/src/jwt/issuer_index.rs`:
- Around line 45-59: find_token_metadata_key currently scans self.index
(accessed via index.read()) and will return the first token_key whose
policy.token_metadata.entity_type_name matches, which can hide duplicate
entity_type_name across issuers; add a validation step in the policy store
loader (the code that populates self.index/issuer_config) to reject or error on
duplicate entity_type_name values across different issuers by tracking seen
entity_type_names while iterating issuer configs and failing the load if a
duplicate is detected, or alternatively log a clear, documented error including
the conflicting issuer identifiers so the invariant is enforced rather than
silently relying on namespace conventions.

Comment on lines 117 to 118
let trusted_issuer_workers = NonZeroUsize::new(raw.trusted_issuer_loader_workers)
.unwrap_or_else(|| unsafe { NonZeroUsize::new_unchecked(1) });
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

🌐 Web query:

Rust NonZeroUsize::new safe alternative to new_unchecked best practice

💡 Result:

The safe alternative to NonZeroUsize::new_unchecked(x) is almost always:

use std::num::NonZeroUsize;

let nz = NonZeroUsize::new(x); // -> Option<NonZeroUsize>

new_unchecked is unsafe because passing 0 violates NonZero*’s “never zero” validity invariant and is undefined behavior (UB). The standard library explicitly documents this: new returns Option after checking, while new_unchecked “results in undefined behavior if the value is zero.” [1]

Best-practice patterns

1) Prefer new + handle Option

let nz = NonZeroUsize::new(x).ok_or(MyError::Zero)?;

(or if let Some(nz) = ... / match).

2) If “zero is a bug” at a boundary, use expect/unwrap

let nz = NonZeroUsize::new(x).expect("x must be non-zero");

This is also usable in const contexts because Option::expect/unwrap is const (shown in the std docs for NonZero*). [2]

3) Only use new_unchecked when you prove non-zero and need it

Typical justification: you already performed a check and want to avoid a redundant branch in a hot path:

let nz = match NonZeroUsize::new(x) {
    Some(v) => v,
    None => return Err(MyError::Zero),
};

// later, if you must reconstruct and can prove non-zero:
let y = nz.get();              // guaranteed non-zero by the type invariant [3]
let nz2 = unsafe { NonZeroUsize::new_unchecked(y) }; // usually unnecessary

If you do use new_unchecked, put a clear // SAFETY: x != 0 because ... comment, and consider a debug_assert!(x != 0);.

Why the invariant matters: NonZero* enables layout optimizations (e.g., Option<NonZero*> can be the same size as the integer type, using 0 as the niche for None). [3], [4]

Sources:
[1] Rust std docs: NonZero::new and NonZero::new_unchecked safety/UB notes https://doc.rust-lang.org/beta/std/num/struct.NonZero.html
[2] Rust std docs: compile-time creation via NonZero*::new(...).expect(...) (example shown for NonZeroIsize) https://doc.rust-lang.org/std/num/type.NonZeroIsize.html
[3] Rust std docs: NonZeroIsize layout/validity + Option<NonZeroIsize> niche optimization explanation https://doc.rust-lang.org/std/num/type.NonZeroIsize.html
[4] Rust forum explanation of why new_unchecked must be unsafe (niche optimization and invariant) https://users.rust-lang.org/t/why-is-std-new-unchecked-unsafe/23151


Remove unnecessary unsafe when defaulting worker count.

The unsafe { NonZeroUsize::new_unchecked(1) } is unnecessary since 1 is compile-time guaranteed to be non-zero. Use the safe constructor instead:

NonZeroUsize::new(1).expect("1 is non-zero")

This follows Rust best practices and avoids unsafe code when the invariant is trivially satisfied.

🤖 Prompt for AI Agents
In `@jans-cedarling/cedarling/src/bootstrap_config/decode.rs` around lines 117 -
118, Replace the unsafe construction for trusted_issuer_workers: instead of
using NonZeroUsize::new_unchecked(1) in the unwrap_or_else closure, call the
safe constructor NonZeroUsize::new(1).expect("1 is non-zero") so the fallback
remains a guaranteed non-zero value without unsafe code; update the closure
where trusted_issuer_workers is assigned (the expression using
raw.trusted_issuer_loader_workers and NonZeroUsize::new_unchecked).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is OK in this case. In this case it is safe

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olehbozhok, understood! While the safe constructor would avoid unsafe with zero runtime cost, I respect your decision to keep it as is.


🧠 Learnings used
Learnt from: olehbozhok
Repo: JanssenProject/jans PR: 13125
File: jans-cedarling/cedarling/src/jwt/mod.rs:177-186
Timestamp: 2026-02-02T18:28:01.853Z
Learning: In jans-cedarling JWT service (file: jans-cedarling/cedarling/src/jwt/mod.rs), the local JWKS loading is intentionally gated by `loader.jwt_config.trusted_issuer_loader.is_sync()`. This means local JWKS are only loaded in synchronous loader mode, not in async mode. This is by design.

Learnt from: olehbozhok
Repo: JanssenProject/jans PR: 12896
File: jans-cedarling/cedarling/src/entity_builder/trusted_issuer_index.rs:23-30
Timestamp: 2025-12-24T14:16:17.748Z
Learning: In jans-cedarling Rust code targeting WASM, avoid using `eprintln!` for warnings in constructors or initialization functions. Instead, accept an `Option<&Arc<LogStrategy>>` parameter and use `logger.log_any(LogEntry::new_with_data(LogType::System, None).set_message(...).set_log_level(LogLevel::WARN))` to emit warnings that work in WASM environments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should refactor this i dont see the point of using unnecessary unsafe when it could be done safely and i think we can also use .unwrap_or(NonZeroUsize::MIN) // MIN is always 1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should also add input validation for worker count NonZeroUsize prevents zero, but no upper bound check exists. A misconfigured value (e.g., 10000) could spawn excessive tasks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should refactor this i dont see the point of using unnecessary unsafe when it could be done safely and i think we can also use .unwrap_or(NonZeroUsize::MIN) // MIN is always 1

I agree with @haileyesus2433 with this unless there is an explicit reason to use unsafe we should avoid it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using NonZeroUsize::MIN is a really good idea. Thanks!

Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
jans-cedarling/cedarling/src/jwt/validation/validator_cache.rs (1)

303-330: ⚠️ Potential issue | 🟡 Minor

Add a negative lookup case and use matches! with an explicit message.

The test only asserts the positive path. Please add a negative case and use assert!(matches!(...), "...") for clarity.

✅ Suggested test update
         store.insert(&info, validator.clone());

-        assert!(store.get(&info).is_some());
+        assert!(
+            matches!(store.get(&info), Some(_)),
+            "retrieves inserted validator"
+        );
+
+        let missing_info = ValidatorInfo {
+            iss: Some(&test_iss),
+            token_kind: TokenKind::AuthzRequestInput("missing_tkn"),
+            algorithm: jsonwebtoken::Algorithm::HS256,
+        };
+        assert!(
+            matches!(store.get(&missing_info), None),
+            "returns None for unknown validator"
+        );
As per coding guidelines: Include both positive and negative test cases using assert!(matches!(...), "explicit comment") instead of assert!(result.is_err()).
🤖 Fix all issues with AI agents
In `@jans-cedarling/cedarling/src/jwt/validation/validator_cache.rs`:
- Around line 21-23: The code currently panics on poisoned RwLock via
EXPECT(MUTEX_POISONED_ERR); define a thiserror enum ValidatorCacheError (with
LockPoisoned variant) and change JwtValidatorCache methods init_for_iss, insert,
and get to return Result types (init_for_iss -> Result<(), ValidatorCacheError>,
insert -> Result<(), ValidatorCacheError>, get ->
Result<Option<Arc<RwLock<JwtValidator>>>, ValidatorCacheError>); replace
.expect(MUTEX_POISONED_ERR) with .map_err(|_|
ValidatorCacheError::LockPoisoned)? to convert poison into an error and
propagate Ok(()) from init_for_iss; update the callsite in load_trusted_issuer
to propagate the new error into JwtServiceInitError as required.

…R_LOADER_WORKERS` property

Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@jans-cedarling/cedarling/src/bootstrap_config/raw_config/config.rs`:
- Around line 309-330: Add unit tests in the config.rs test module covering the
new fields: verify trusted_issuer_loader_type (TrustedIssuerLoaderTypeRaw) and
trusted_issuer_loader_workers (usize) default values when unset, that
environment variables CEDARLING_TRUSTED_ISSUER_LOADER_TYPE and
CEDARLING_TRUSTED_ISSUER_LOADER_WORKERS are parsed correctly (via serde with
deserialize_or_parse_string_as_json), and that JSON deserialization accepts
valid values and rejects invalid ones; specifically add tests that (1) construct
the config with no env/JSON and assert defaults, (2) set the env vars and assert
the resulting trusted_issuer_loader_type and trusted_issuer_loader_workers, and
(3) pass JSON strings for those fields including invalid inputs to assert
deserialization errors.

…DER` parameters

Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@jans-cedarling/cedarling/src/bootstrap_config/raw_config/config.rs`:
- Around line 309-330: The docstring for trusted_issuer_loader_workers is
ambiguous about the default: update the comment for the pub
trusted_issuer_loader_workers: usize field to clearly state that although the
serde/default value may be zero when the env var is unset or parsed as 0, the
effective runtime default used by the loader logic is 1 (i.e., "Default value is
1; when unset or parsed as 0 it will be treated as 1"), so readers immediately
understand zero is treated as the default without needing to read the next
sentence; keep the serde attributes and type unchanged and only modify the
comment for clarity.

@olehbozhok
Copy link
Contributor Author

@coderabbitai looks like all comments resolved. Maybe you can approve the PR?

coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 2, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 2, 2026

@olehbozhok, thank you for addressing all the review comments! I'll approve the PR now.

    (\__/)
    (='.'=)  Great work on implementing the background
    (")_(")  trusted issuer loading feature!

🧠 Learnings used
Learnt from: moabu
Repo: JanssenProject/jans PR: 13060
File: CHANGELOG.md:25-26
Timestamp: 2026-01-22T14:33:53.554Z
Learning: Repo: JanssenProject/jans — For Release Please-generated PRs (chore(main): release …), CHANGELOG.md is auto-generated; avoid suggesting manual edits (e.g., duplicate/formatting cleanups) and ignore minor nits in that file.

Learnt from: olehbozhok
Repo: JanssenProject/jans PR: 13125
File: jans-cedarling/cedarling/src/jwt/mod.rs:177-186
Timestamp: 2026-02-02T18:28:01.853Z
Learning: In jans-cedarling JWT service (file: jans-cedarling/cedarling/src/jwt/mod.rs), the local JWKS loading is intentionally gated by `loader.jwt_config.trusted_issuer_loader.is_sync()`. This means local JWKS are only loaded in synchronous loader mode, not in async mode. This is by design.

Learnt from: olehbozhok
Repo: JanssenProject/jans PR: 13024
File: jans-cedarling/cedarling/src/entity_builder/build_multi_issuer_entity.rs:233-244
Timestamp: 2026-01-16T14:05:12.899Z
Learning: In jans-cedarling multi-issuer entity building (file: jans-cedarling/cedarling/src/entity_builder/build_multi_issuer_entity.rs), reserved claims (token_type, jti, iss, exp, validated_at) are marked as optional in the Cedar schema (with `?`) but documented as "Required for multi-issuer" in comments. This schema design allows the code to filter these claims out during initial attribute building (via `filter_reserved_claims`) and add them separately afterward without causing Cedar schema validation failures for missing required attributes.

Learnt from: olehbozhok
Repo: JanssenProject/jans PR: 13125
File: jans-cedarling/cedarling/src/common/policy_store.rs:314-316
Timestamp: 2026-02-02T18:17:53.167Z
Learning: In jans-cedarling `TrustedIssuer.iss_claim()` (file: jans-cedarling/cedarling/src/common/policy_store.rs), using `Url::origin().ascii_serialization()` to derive the issuer claim from the OIDC endpoint is intentional design. The method is meant for issuer comparison and uses origin-only, while `get_oidc_endpoint()` preserves the full URL for making requests. The primary issuer value comes from the fetched OpenID configuration response, not from this method.
✅ Actions performed

Comments resolved and changes approved.

Copy link
Contributor

@haileyesus2433 haileyesus2433 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct me if am wrong @olehbozhok but this PR implements async/sync loading modes but NOT true lazy loading as described in the issue. The issue states:

"Implement lazy loading of the JWKS endpoint until the Cedarling sees a token with the respective issuer"

but current implementation still loads ALL issuers at startup (either synchronously or in the background).
True lazy loading would:

  • NOT fetch JWKS until a token from that issuer is encountered
  • Trigger on-demand fetching when validating a token from an unloaded issuer

there is a misalignment with the issue and the PR

@@ -0,0 +1,792 @@
// This software is available under the Apache-2.0 license.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using .expect() in concurrent code can cause panics. If the semaphore fails or mutex is poisoned, the entire service crashes. we should replace with proper error handling

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "proper error handling`?
Just return an error and propagate it? Or try to run the process of fetching data again?
Or maybe anything other?

From my point of view, if the mutex is poisoned or the semaphore is already closed, this is a critical error.
Because we made a logical error in how to work with workers. Probably, we can log this, but nothing more. We have no guarantee that the data that we have is valid or consistent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes i think we if we add an Exponential Backoff strategy, then if it still fails we should log it instead of crashing the whole thing, i think they should be able to continue with loaded trusted issuers instead of crashing


loader.check_keys_loaded();

let errors = Arc::into_inner(errors)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think Arc::into_inner() fails silently and If other references exist, Arc::into_inner() returns None and will panic. i think we should use Arc::try_unwrap() with proper error handling.

);
errors_clone
.lock()
.expect("failed to lock errors mutex while recording issuer loading failure - mutex may be poisoned due to panic in another thread")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a thread panics while holding the mutex, subsequent operations will fail. i think we should find ways recover from the poisoning

let errors = Arc::into_inner(errors)
.expect("failed to extract errors Arc - other references may still exist indicating concurrent access during issuer loading")
.into_inner()
.expect("failed to extract errors from Mutex - mutex may be poisoned due to panic while holding lock");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same issue we should find ways to recover from poison

Comment on lines 117 to 118
let trusted_issuer_workers = NonZeroUsize::new(raw.trusted_issuer_loader_workers)
.unwrap_or_else(|| unsafe { NonZeroUsize::new_unchecked(1) });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should refactor this i dont see the point of using unnecessary unsafe when it could be done safely and i think we can also use .unwrap_or(NonZeroUsize::MIN) // MIN is always 1

Comment on lines 117 to 118
let trusted_issuer_workers = NonZeroUsize::new(raw.trusted_issuer_loader_workers)
.unwrap_or_else(|| unsafe { NonZeroUsize::new_unchecked(1) });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should also add input validation for worker count NonZeroUsize prevents zero, but no upper bound check exists. A misconfigured value (e.g., 10000) could spawn excessive tasks.

Ok(())
}

#[cfg(test)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are critical test cases missing like

  1. No test for true lazy loading

    • Missing: Test that JWKS is fetched when token with unloaded issuer arrives
    • Missing: Test that validation waits for lazy load to complete
  2. No test for race conditions

    • Missing: Test concurrent token validation during async loading
    • Missing: Test multiple tokens from same issuer during loading
  3. No test for WASM compatibility

    • Missing: WASM-specific test cases
    • Missing: Test spawn_task behavior in WASM
  4. No test for error recovery

    • Missing: Test behavior when JWKS fetch fails after retry
    • Missing: Test mutex poisoning recovery
  5. No test for worker limit

    • Missing: Test behavior with maximum workers
    • Missing: Test behavior with single worker and many issuers
  6. No integration test for async mode timing

    • Missing: End-to-end test showing async loading doesn't block startup
    • Missing: Test that service accepts requests while loading in background

let mut issuer_configs = HashMap::default();
let mut validators = JwtValidatorCache::default();
let mut key_service = KeyService::new();
let status_lists = Arc::new(StatusListCache::default());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question here why wrap StatusListCache in Arc when the field in this struct is already wrapped in Arc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, fixed 7ede7b2

/// Verifies that when `jwt_status_validation` is true, the status list cache is initialized
/// for the issuer and contains the expected status list.
#[tokio::test]
#[ignore = "Mock server issue with status list endpoint"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we ignore this test then what use does it have?

@olehbozhok
Copy link
Contributor Author

correct me if am wrong @olehbozhok but this PR implements async/sync loading modes but NOT true lazy loading as described in the issue. The issue states:

"Implement lazy loading of the JWKS endpoint until the Cedarling sees a token with the respective issuer"

but current implementation still loads ALL issuers at startup (either synchronously or in the background). True lazy loading would:

  • NOT fetch JWKS until a token from that issuer is encountered
  • Trigger on-demand fetching when validating a token from an unloaded issuer

there is a misalignment with the issue and the PR

I was thinking a lot about that. The main point of authorisation is that it should be incredibly fast. Any delay may cause a freeze of the UI application, which we would like to avoid.
After communication with Michael, he agreed with me that if a user doesn't want to load all trusted issuers on start better to load them in the background.

In a usual use case, we have a small amount of trusted issuers. (it's words of Michael)

@haileyesus2433
Copy link
Contributor

haileyesus2433 commented Feb 4, 2026

correct me if am wrong @olehbozhok but this PR implements async/sync loading modes but NOT true lazy loading as described in the issue. The issue states:

"Implement lazy loading of the JWKS endpoint until the Cedarling sees a token with the respective issuer"

but current implementation still loads ALL issuers at startup (either synchronously or in the background). True lazy loading would:

  • NOT fetch JWKS until a token from that issuer is encountered
  • Trigger on-demand fetching when validating a token from an unloaded issuer

there is a misalignment with the issue and the PR

I was thinking a lot about that. The main point of authorisation is that it should be incredibly fast. Any delay may cause a freeze of the UI application, which we would like to avoid. After communication with Michael, he agreed with me that if a user doesn't want to load all trusted issuers on start better to load them in the background.

In a usual use case, we have a small amount of trusted issuers. (it's words of Michael)

ok well in that case i think the issue should be updated then so that it aligns with the PR dont u think so @olehbozhok?

Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
@olehbozhok olehbozhok changed the title feat (jans-cedarling): Load trusted issuers on startup in the background feat (jans-cedarling): Load trusted issuers on startup on the background Feb 9, 2026
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-documentation Documentation needs to change as part of issue or PR comp-docs Touching folder /docs comp-jans-cedarling Touching folder /jans-cedarling kind-feature Issue or PR is a new feature request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat (jans-cedarling): Background loading of Trusted IDP metadata

4 participants